install/proxmox/install/netalertx-install.sh (1)

237-246: Deduplicate NETALERTX_TMP/PORT exports.

The repeated lines add noise without changing behavior.

🧹 Suggested cleanup

 export NETALERTX_TMP=/app
 export PORT=${PORT}
-export NETALERTX_TMP=/app
-export PORT=${PORT}
 export PYTHONPATH=/app

install/proxmox/proxmox-install-netalertx.sh (2)

336-343: Environment variables are duplicated between start script and systemd service.

The NETALERTX_* environment variables are defined in both the generated start script (lines 336-343) and the systemd service file (lines 386-393). While this redundancy ensures the variables are set regardless of how the script is invoked, consider consolidating to a single source of truth (preferably the systemd service) to simplify maintenance.

Also applies to: 386-393

---

345-348: Self-healing __init__.py creation may mask packaging issues.

The __init__.py files for /app/front/ and /app/front/plugins/ do not exist in the repository but are being created at startup. This defensive measure could hide legitimate packaging problems. These files should either be committed to the repository as part of proper package structure, or their creation should be removed if they're genuinely not needed for the application's import structure.

install/proxmox/install/netalertx-install.sh (3)

174-176: Unquoted brace expansion in touch command.

Line 175 uses brace expansion ${INSTALL_DIR}/log/{app.log,...} which works, but if any path component contained spaces, it would break. More importantly, if sudo -u www-data touch fails (e.g., permission issue), the error is silently swallowed since there's no error checking.

This is a minor robustness concern given the controlled environment.

---

267-300: Systemd service configuration is reasonable but consider adding security hardening directives.

The service runs as www-data which is good. However, for a network monitoring tool, consider adding systemd sandboxing directives to limit the blast radius (e.g., ProtectSystem=, ProtectHome=, NoNewPrivileges=). NoNewPrivileges=true would conflict with setcap/sudo usage, so that needs careful thought, but directives like ProtectHome=yes are safe here.

---

22-58: Large dependency list — consider documenting why each package is needed.

This installs ~30 packages. A brief inline comment grouping them by purpose (e.g., # Network scanning tools, # Build tools, # Runtime) would help future maintainers understand which are essential vs. optional.

Minor: tini is typically used as a Docker init process; it's unusual in a systemd-managed LXC container. Is it actually needed here?

install/proxmox/ct/netalertx.sh (3)

65-101: Bridge detection logic is thorough with good fallbacks.

The multi-method approach (ip link → sysfs → pvesh) with user prompting for multiple bridges is well thought out. The error handling for no-bridge-found is appropriate.

One minor note: line 69 concatenates two sources without deduplication by interface type — ip -o link show type bridge could return non-vmbr bridges (e.g., docker0, br-*). The grep vmbr on the sysfs path filters correctly, but the ip command output isn't filtered. The sort -u dedup on line 72 handles exact duplicates but not irrelevant bridges.

Consider filtering ip output to vmbr* bridges only

-  BRIDGES=($(ip -o link show type bridge | awk -F': ' '{print $2}') $(ls /sys/class/net | grep vmbr | grep -v "vmbr0"))
+  BRIDGES=($(ip -o link show type bridge | awk -F': ' '{print $2}' | grep '^vmbr') $(ls /sys/class/net 2>/dev/null | grep '^vmbr' | grep -v '^vmbr0$'))

---

12-13: Sourcing remote code from an unpinned branch over HTTPS.

source <(curl -fsSL https://...ProxmoxVE/raw/main/misc/build.func) fetches and executes arbitrary code from main with no integrity verification. This is standard practice for Proxmox community scripts, but be aware that any upstream compromise or breaking change will silently propagate. Consider pinning to a commit SHA for production use.

---

56-57: Exporting REPO_URL without validation.

REPO_URL is exported and eventually consumed by git clone in the install script. If a user sets REPO_URL to a value containing shell metacharacters, it flows through unchecked. The install script side was noted separately, but the export here is the origin point.

install/proxmox/install/netalertx-install.sh (2)

234-263: Unquoted heredoc expands ${SERVER_IP} and ${PORT} at install time.

Since <<EOF (not <<'EOF') is used, these values are baked into the startup script. If the container's IP changes (e.g., DHCP), the banner message will be stale. The environment variable PORT is also expanded, so changing it later requires editing the script. This is likely intentional for a static LXC setup, but worth a comment in the script for maintainers.

---

267-299: Environment variables are set both in the systemd unit and in start.netalertx.sh.

The Environment= directives in the service file (lines 285–291) duplicate the exports in the startup script (lines 239–246). The startup script's exports will take precedence at runtime, making the systemd entries effectively dead config. Consider keeping them in only one place to avoid drift.

install/proxmox/ct/netalertx.sh (1)

40-47: ⚠️ Potential issue | 🟠 Major

build_container rewriting via sed+eval is too fragile without post-checks.

If upstream build_container changes format, these substitutions can no-op and the script continues with unintended behavior.

🔍 Suggested hardening

   eval "$(echo "$modified_func" | sed "s|lxc-attach|${export_header} lxc-attach|g; s|pct exec|${export_header} pct exec|g")"
+  new_func=$(declare -f build_container)
+  if ! grep -Fq "${REPOS_URL}/install/proxmox/install/\${var_install}-install.sh" <<< "$new_func"; then
+    msg_error "Failed to override build_container for REPOS_URL=${REPOS_URL}"
+    exit 1
+  fi
 fi
 ...
   eval "$(echo "$original_func" | sed "s|lxc-attach.*install/\${var_install}.sh.*|$replacement|")"
+  new_func=$(declare -f build_container)
+  if ! grep -Fq '/root/install.sh' <<< "$new_func"; then
+    msg_error "Failed to override build_container with LOCAL_INSTALLER=${LOCAL_INSTALLER}"
+    exit 1
+  fi
 fi

Also applies to: 55-58

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@install/proxmox/ct/netalertx.sh` around lines 40 - 47, The sed+eval rewrite
of build_container is fragile and may silently no-op if upstream changes; modify
the flow to (1) verify the original_func contains the expected call sites
("lxc-attach" or "pct exec") before attempting substitions, (2) perform
substitutions into modified_func as now, and then (3) assert that the resulting
modified_func actually contains the injected export header string
(export_header) adjacent to the exec call and the replaced REPOS_URL pattern; if
any check fails, print a clear error and abort instead of eval-ing, using the
existing build_container symbol names (original_func, modified_func,
export_header, lxc-attach, pct exec) so the script fails fast when rewrites did
not apply.

install/proxmox/install/netalertx-install.sh (2)

217-219: ⚠️ Potential issue | 🟠 Major

Restrict /app permissions; Line 218 reintroduces a world-writable install tree.

chmod -R a+rwx "$INSTALL_DIR" allows any local user/process to modify app code, config, and DB.

🔒 Suggested fix

 chgrp -R www-data "$INSTALL_DIR"
-# NetAlertX needs write access to front/ for some features, and broad access to /app
-chmod -R a+rwx "$INSTALL_DIR"
+# Keep write access to owner/group only
+chmod -R ug+rwX,o-rwx "$INSTALL_DIR"
+# Ensure group inheritance for newly created directories/files
+find "$INSTALL_DIR" -type d -exec chmod g+s {} +
 chown -R www-data:www-data "${INSTALL_DIR}/db/app.db"

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@install/proxmox/install/netalertx-install.sh` around lines 217 - 219, The
current chmod -R a+rwx "$INSTALL_DIR" makes the whole install tree
world-writable; replace this with a minimum-privilege approach: ensure the app
tree is owned by the web user (use the existing chown for the DB and extend to
"${INSTALL_DIR}"), set directory permissions to 755 and regular file permissions
to 644 (or use u=rwX,go=rX semantics) and set the DB file to 660 owned by
www-data:www-data; specifically update the chmod/chown usage around INSTALL_DIR
and the existing chown for "${INSTALL_DIR}/db/app.db" to apply these tighter
permissions instead of a+rwx.

---

165-172: ⚠️ Potential issue | 🟠 Major

PHP-FPM socket handling can still leave NGINX pointed at a stale upstream.

When the socket is absent at Line 167 (common before php-fpm starts), the script only warns, then never reapplies socket replacement after Line 252 starts php-fpm.

🔧 Suggested fix

-# Postpone PHP-FPM socket detection until after service start, or use a fallback.
-# For now, we configure a default and assume the standard Debian 13/Ubuntu 24 location.
-if [ -S "/run/php/php8.4-fpm.sock" ]; then
-    sed -i "s|unix:/var/run/php/php-fpm.sock;|unix:/run/php/php8.4-fpm.sock;|g" /etc/nginx/conf.d/netalertx.conf
-else
-    # Fallback pattern for detection during startup if possible
-    msg_warn "PHP-FPM socket not found at standard location, will rely on service startup"
-fi
+# Use deterministic socket path for this installer target
+sed -i "s|unix:/var/run/php/php-fpm.sock;|unix:/run/php/php8.4-fpm.sock;|g" /etc/nginx/conf.d/netalertx.conf
...
 systemctl enable php8.4-fpm
 systemctl start php8.4-fpm
+systemctl restart nginx
 msg_ok "Started PHP-FPM"

Also applies to: 250-253

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@install/proxmox/install/netalertx-install.sh` around lines 165 - 172, The
current logic updates /etc/nginx/conf.d/netalertx.conf only if
/run/php/php8.4-fpm.sock exists and otherwise just msg_warns, which can leave
NGINX pointed at a stale upstream; after the script starts PHP-FPM (the block
that starts php-fpm around the later start/daemon step), re-check for the socket
and re-run the same sed replacement used earlier (the sed
"s|unix:/var/run/php/php-fpm.sock;|unix:/run/php/php8.4-fpm.sock;|g" against
netalertx.conf) and then reload nginx (systemctl reload nginx or nginx -s
reload) so the replacement is applied when the socket appears—add this
retry/check immediately after the php-fpm start block and keep the msg_warn for
the initial absence.

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

install/proxmox/ct/netalertx.sh (3)

66-67: ⚠️ Potential issue | 🔴 Critical

Same command injection vulnerability in local installer override.

The replacement variable also interpolates unescaped REPOS_URL, REPO_URL, and REPO_BRANCH into an eval context.

🔒 Proposed fix

   msg_info "Using local installer from $LOCAL_INSTALLER"
   original_func=$(declare -f build_container)
-  replacement="export REPOS_URL=${REPOS_URL}; export REPO_URL=${REPO_URL}; export REPO_BRANCH=${REPO_BRANCH}; pct push \"\$CTID\" \"$LOCAL_INSTALLER\" /root/install.sh && lxc-attach -n \"\$CTID\" -- bash /root/install.sh"
+  printf -v q_REPOS_URL '%q' "${REPOS_URL:-}"
+  printf -v q_REPO_URL '%q' "${REPO_URL:-}"
+  printf -v q_REPO_BRANCH '%q' "${REPO_BRANCH:-}"
+  replacement="export REPOS_URL=${q_REPOS_URL}; export REPO_URL=${q_REPO_URL}; export REPO_BRANCH=${q_REPO_BRANCH}; pct push \"\$CTID\" \"$LOCAL_INSTALLER\" /root/install.sh && lxc-attach -n \"\$CTID\" -- bash /root/install.sh"
   eval "$(echo "$original_func" | sed "s|lxc-attach.*install/\${var_install}.sh.*|$replacement|")"

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@install/proxmox/ct/netalertx.sh` around lines 66 - 67, The replacement string
assigned to replacement and then injected into eval (via eval "$(echo
"$original_func" | sed ... )") is vulnerable because REPOS_URL, REPO_URL and
REPO_BRANCH are interpolated unescaped; fix by stopping direct interpolation
into the eval string: build the command without expanding those variables (use
quoted placeholders) or use a safe-quoting mechanism (e.g. shell-quote/printf
%q) when composing replacement, and update the sed substitution that references
original_func so it inserts the safely-quoted/placeholdered values instead of
raw REPOS_URL/REPO_URL/REPO_BRANCH; ensure the final command executed by eval
only receives properly quoted values or, better, avoid eval and call
pct/lxc-attach with arguments assembled as arrays or via a here-doc to eliminate
command injection risks.

---

60-60: ⚠️ Potential issue | 🟠 Major

LOCAL_INSTALLER path resolves incorrectly.

From install/proxmox/ct/, going ../../install/ lands at install/install/, not install/proxmox/install/. The local installer override will never trigger because the file won't be found at the wrong path.

🔧 Proposed fix

-LOCAL_INSTALLER="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)/../../install/${NSAPP:-netalertx}-install.sh"
+LOCAL_INSTALLER="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)/../install/${NSAPP:-netalertx}-install.sh"

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@install/proxmox/ct/netalertx.sh` at line 60, The LOCAL_INSTALLER path in
install/proxmox/ct/netalertx.sh is resolving to the wrong directory
(../../install/... yields install/install/) so the override never finds the
installer; update the assignment for LOCAL_INSTALLER to point to the proxmox
install directory (use ../install/${NSAPP:-netalertx}-install.sh) by changing
the path expression around the LOCAL_INSTALLER variable in netalertx.sh so it
looks up one directory (../install) instead of two (../../install).

---

53-56: ⚠️ Potential issue | 🔴 Critical

Unescaped environment variables in eval context enable command injection.

REPOS_URL, REPO_URL, and REPO_BRANCH are interpolated directly into strings passed to eval. A crafted value like "; malicious_command && # breaks out of the export statement and executes arbitrary shell code on the Proxmox host.

🔒 Proposed fix using printf escaping

 if [[ -n "${REPOS_URL}" ]]; then
   # Only show info message in verbose mode to avoid UI overlap
   if [[ "${VERBOSE:-no}" == "yes" ]]; then
     msg_info "Using custom repository: ${REPOS_URL}"
   fi
   # Override build_container to pass environment variables and use the custom repo URL
   original_func=$(declare -f build_container)
-  # Prepend exports to the execution command within build_container
-  # This ensures REPOS_URL, REPO_URL, and REPO_BRANCH are available inside the container during install.
-  export_header="export REPOS_URL=${REPOS_URL}; export REPO_URL=${REPO_URL}; export REPO_BRANCH=${REPO_BRANCH};"
+  # Safely escape variables before interpolation
+  printf -v q_REPOS_URL '%q' "${REPOS_URL:-}"
+  printf -v q_REPO_URL '%q' "${REPO_URL:-}"
+  printf -v q_REPO_BRANCH '%q' "${REPO_BRANCH:-}"
+  export_header="export REPOS_URL=${q_REPOS_URL}; export REPO_URL=${q_REPO_URL}; export REPO_BRANCH=${q_REPO_BRANCH};"
   modified_func=$(echo "$original_func" | sed "s|https://raw.githubusercontent.com/community-scripts/ProxmoxVE/main/install/\${var_install}.sh|${REPOS_URL}/install/proxmox/install/\${var_install}-install.sh|g")
   # Inject the exports before the lxc-attach/pct exec call
   eval "$(echo "$modified_func" | sed "s|lxc-attach|${export_header} lxc-attach|g; s|pct exec|${export_header} pct exec|g")"
 fi

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@install/proxmox/ct/netalertx.sh` around lines 53 - 56, The code builds
export_header and modified_func by directly interpolating
REPOS_URL/REPO_URL/REPO_BRANCH and then passes the result to eval, which allows
command injection; fix by first escaping those env values (e.g., use printf %q
into safe_REPOS_URL, safe_REPO_URL, safe_REPO_BRANCH) and use the escaped
variables when constructing export_header and when performing the sed
replacements for modified_func, and eliminate the unsafe eval by writing
modified_func to a temp file and sourcing it or invoking it with bash -c with
the function body passed as a single safe argument; update the usages of
export_header, modified_func, original_func and the eval call accordingly.

install/proxmox/ct/netalertx.sh (1)

93-93: Consider using jq for JSON parsing if available.

The grep -oP pattern for extracting bridge names from JSON is fragile if the output format changes. Since jq is used elsewhere in the script ecosystem and is commonly available on Proxmox, it would be more robust.

♻️ Proposed alternative with jq fallback

     if command -v pvesh >/dev/null 2>&1; then
-      # shellcheck disable=SC2207,SC2046  # Working pattern for pvesh output
-      BRIDGES=($(pvesh get /nodes/$(hostname)/network --type bridge --output-format json | grep -oP '"iface":"\K[^"]+'))
+      # shellcheck disable=SC2207,SC2046  # Working pattern for pvesh output
+      if command -v jq >/dev/null 2>&1; then
+        BRIDGES=($(pvesh get /nodes/$(hostname)/network --type bridge --output-format json | jq -r '.[].iface'))
+      else
+        BRIDGES=($(pvesh get /nodes/$(hostname)/network --type bridge --output-format json | grep -oP '"iface":"\K[^"]+'))
+      fi
     fi

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@install/proxmox/ct/netalertx.sh` at line 93, Replace the fragile grep
extraction used when setting BRIDGES by using jq if available: detect jq with a
command -v check, then run the existing pvesh get /nodes/$(hostname)/network
--output-format json and pipe to jq -r '.[].iface' (or the appropriate JSON
path) to populate the BRIDGES array; if jq is missing fall back to the current
grep -oP '"iface":"\K[^"]+' pipeline. Update the BRIDGES assignment and any
error handling so the script uses the jq result when present and the grep
fallback otherwise.

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

install/proxmox/ct/netalertx.sh (3)

32-36: ⚠️ Potential issue | 🟠 Major

Add a post-override assertion so function patching cannot fail silently.

At Line 35 and Line 46, build_container is rewritten by pattern-matching its body. If upstream formatting changes, the rewrite can no-op and continue with unintended install behavior. Fail fast after each override.

Suggested hardening

 if [[ -n "${REPOS_URL}" ]]; then
   # Override build_container surgically by replacing ONLY the URL string.
   # This is much safer than re-declaring the whole function.
   source <(declare -f build_container | sed "s|https://raw.githubusercontent.com/community-scripts/ProxmoxVE/main/install/\${var_install}.sh|${REPOS_URL}/install/proxmox/install/\${var_install}-install.sh|g")
+  if ! declare -f build_container | grep -Fq '/install/proxmox/install/${var_install}-install.sh'; then
+    msg_error "Failed to override build_container for REPOS_URL=${REPOS_URL}"
+    exit 1
+  fi
 fi
@@
 if [[ -f "$LOCAL_INSTALLER" ]]; then
   msg_info "Using local installer from $LOCAL_INSTALLER"
   original_func=$(declare -f build_container)
   replacement="export REPOS_URL=${REPOS_URL}; export REPO_URL=${REPO_URL}; export REPO_BRANCH=${REPO_BRANCH}; pct push \"\$CTID\" \"$LOCAL_INSTALLER\" /root/install.sh && lxc-attach -n \"\$CTID\" -- bash /root/install.sh"
   eval "$(echo "$original_func" | sed "s|lxc-attach.*install/\${var_install}.sh.*|$replacement|")"
+  if ! declare -f build_container | grep -Fq '/root/install.sh'; then
+    msg_error "Failed to override build_container for LOCAL_INSTALLER=${LOCAL_INSTALLER}"
+    exit 1
+  fi
 fi

Also applies to: 42-47

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@install/proxmox/ct/netalertx.sh` around lines 32 - 36, The override of the
build_container function via source <(declare -f build_container | sed ...) can
silently no-op if the sed pattern fails; after each such override (the blocks
that reference build_container and ${REPOS_URL}/${var_install}), add a
post-override assertion that verifies the function was actually patched—e.g.,
capture declare -f build_container and test that the body contains the new
${REPOS_URL}/install/proxmox/install/${var_install}-install.sh URL (or otherwise
differs from the original), and if the check fails emit a clear error
(mentioning build_container and REPOS_URL/var_install) and exit non-zero so the
script fails fast instead of continuing with an unmodified function.

---

45-46: ⚠️ Potential issue | 🔴 Critical

Escape repo variables before injecting into eval-generated function content.

At Line 45-Line 46, REPOS_URL / REPO_URL / REPO_BRANCH are interpolated unescaped into content later executed via eval. Crafted values can inject commands into the generated function body.

Minimal escaping fix

 if [[ -f "$LOCAL_INSTALLER" ]]; then
   msg_info "Using local installer from $LOCAL_INSTALLER"
   original_func=$(declare -f build_container)
-  replacement="export REPOS_URL=${REPOS_URL}; export REPO_URL=${REPO_URL}; export REPO_BRANCH=${REPO_BRANCH}; pct push \"\$CTID\" \"$LOCAL_INSTALLER\" /root/install.sh && lxc-attach -n \"\$CTID\" -- bash /root/install.sh"
+  printf -v q_REPOS_URL '%q' "${REPOS_URL:-}"
+  printf -v q_REPO_URL '%q' "${REPO_URL:-}"
+  printf -v q_REPO_BRANCH '%q' "${REPO_BRANCH:-}"
+  replacement="export REPOS_URL=${q_REPOS_URL}; export REPO_URL=${q_REPO_URL}; export REPO_BRANCH=${q_REPO_BRANCH}; pct push \"\$CTID\" \"$LOCAL_INSTALLER\" /root/install.sh && lxc-attach -n \"\$CTID\" -- bash /root/install.sh"
   eval "$(echo "$original_func" | sed "s|lxc-attach.*install/\${var_install}.sh.*|$replacement|")"
 fi

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@install/proxmox/ct/netalertx.sh` around lines 45 - 46, The replacement
currently injects unescaped REPOS_URL/REPO_URL/REPO_BRANCH into an
eval-generated function (via original_func, replacement, eval and sed), allowing
command injection; fix by escaping those variables before building replacement
(e.g. compute safe_REPOS_URL=$(printf '%q' "$REPOS_URL") and likewise for
REPO_URL and REPO_BRANCH) and use the escaped names when composing the
replacement string so pct push / lxc-attach receive only quoted/escaped values
instead of raw interpolated input. Ensure you update the code that constructs
replacement (the sed/eval step) to reference the escaped variables rather than
the original ones.

---

39-39: ⚠️ Potential issue | 🟠 Major

Fix LOCAL_INSTALLER path resolution (currently points to install/install/...).

At Line 39, from install/proxmox/ct, using ../../install/... resolves to <repo>/install/install/..., so the local override can be skipped even when the installer exists.

Path fix

-LOCAL_INSTALLER="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)/../../install/${NSAPP:-netalertx}-install.sh"
+LOCAL_INSTALLER="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)/../install/${NSAPP:-netalertx}-install.sh"

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@install/proxmox/ct/netalertx.sh` at line 39, The LOCAL_INSTALLER path wrongly
includes an extra "install" segment causing install/install/...; update the
assignment in install/proxmox/ct/netalertx.sh (the LOCAL_INSTALLER variable) to
point to the correct relative path by removing the redundant "install"
directory, i.e. use ../../${NSAPP:-netalertx}-install.sh so it resolves to
repo/install/<name>-install.sh.

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

install/proxmox/ct/netalertx.sh (4)

111-131: ⚠️ Potential issue | 🟠 Major

Avoid leaving netalertx.service down on update failure.

Line 112 stops the service, but failure paths in Line 121/122/129 exit 1 before restart. Add a trap/finally-style restart guard.

🛟 Proposed fail-safe pattern

 function update_script() {
   header_info
   check_container_storage
   check_container_resources
+  local restart_needed=0
@@
   msg_info "Stopping ${APP} Service"
-  systemctl stop netalertx.service
+  systemctl stop netalertx.service || exit 1
+  restart_needed=1
+  trap '[[ "$restart_needed" -eq 1 ]] && systemctl start netalertx.service >/dev/null 2>&1 || true' EXIT
   msg_ok "Stopped ${APP} Service"
@@
   $STD pip install -r install/proxmox/requirements.txt || exit 1
   deactivate
   msg_ok "Updated Python Dependencies"
@@
   msg_info "Starting ${APP} Service"
   systemctl start netalertx.service
+  restart_needed=0
+  trap - EXIT
   msg_ok "Started ${APP} Service"

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@install/proxmox/ct/netalertx.sh` around lines 111 - 131, The script currently
stops netalertx.service (systemctl stop netalertx.service) early and exits on
failures (git fetch/reset, pip install) which can leave the service down; wrap
the update block with a fail-safe that always attempts to restart the service
(systemctl start netalertx.service) on any exit or error—implement a trap or
finally-style cleanup around the sequence that includes BRANCH detection, git
fetch/reset, venv activation (source /opt/netalertx-env/bin/activate), pip
install ($STD pip install ...), and deactivate so that regardless of which
command fails the trap will restart netalertx.service and return the original
exit code; reference netalertx.service, BRANCH, git fetch/reset, source
/opt/netalertx-env/bin/activate, and $STD pip install when adding the guard.

---

35-39: ⚠️ Potential issue | 🔴 Critical

Escape repo variables before eval to prevent command injection.

Line 35 and Line 48 interpolate REPOS_URL/REPO_URL/REPO_BRANCH directly into strings later executed by eval (Line 38 and Line 50). If those values are attacker-controlled, this is host-side command execution.

🔐 Proposed fix

 if [[ -n "${REPOS_URL}" ]]; then
+  printf -v q_REPOS_URL '%q' "${REPOS_URL:-}"
+  printf -v q_REPO_URL '%q' "${REPO_URL:-}"
+  printf -v q_REPO_BRANCH '%q' "${REPO_BRANCH:-}"
   # Inject exports and override the installer URL surgically.
   # This pattern matches the exact curl -fsSL call in build.func
-  export_header="export REPOS_URL=${REPOS_URL}; export REPO_URL=${REPO_URL}; export REPO_BRANCH=${REPO_BRANCH};"
+  export_header="export REPOS_URL=${q_REPOS_URL}; export REPO_URL=${q_REPO_URL}; export REPO_BRANCH=${q_REPO_BRANCH};"
   original_func=$(declare -f build_container)
   modified_func=$(echo "$original_func" | sed "s|bash -c \"\$(curl -fsSL [^\"]*)|bash -c \"${export_header} \$(curl -fsSL ${REPOS_URL}/install/proxmox/install/\${var_install}-install.sh)|g")
   eval "$modified_func"
 fi
@@
 if [[ -f "$LOCAL_INSTALLER" ]]; then
   msg_info "Using local installer from $LOCAL_INSTALLER"
   original_func=$(declare -f build_container)
-  export_header="export REPOS_URL=${REPOS_URL}; export REPO_URL=${REPO_URL}; export REPO_BRANCH=${REPO_BRANCH};"
+  printf -v q_REPOS_URL '%q' "${REPOS_URL:-}"
+  printf -v q_REPO_URL '%q' "${REPO_URL:-}"
+  printf -v q_REPO_BRANCH '%q' "${REPO_BRANCH:-}"
+  export_header="export REPOS_URL=${q_REPOS_URL}; export REPO_URL=${q_REPO_URL}; export REPO_BRANCH=${q_REPO_BRANCH};"
   replacement="${export_header} pct push \"\$CTID\" \"$LOCAL_INSTALLER\" /root/install.sh && lxc-attach -n \"\$CTID\" -- bash /root/install.sh"
   eval "$(echo "$original_func" | sed "s|lxc-attach.*install/\${var_install}.sh.*|$replacement|")"
 fi

#!/bin/bash
set -euo pipefail
file="install/proxmox/ct/netalertx.sh"

echo "Inspecting eval-related interpolation:"
nl -ba "$file" | sed -n '32,52p'

echo
echo "Checking whether shell-escaping is used before eval:"
rg -n 'printf -v q_REPOS_URL|printf -v q_REPO_URL|printf -v q_REPO_BRANCH|export_header=.*REPOS_URL=.*REPO_URL=.*REPO_BRANCH=.*|eval "\$modified_func"|eval "\$\(echo \"\$original_func\" \| sed' "$file"

Also applies to: 48-50

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@install/proxmox/ct/netalertx.sh` around lines 35 - 39, The export_header and
modified_func interpolation (variables export_header, original_func,
modified_func, and the later eval of modified_func inside build_container)
inject unescaped REPOS_URL/REPO_URL/REPO_BRANCH into code passed to eval; fix by
shell-escaping/quoting those variables before embedding them (e.g., create
escaped versions like q_REPOS_URL/q_REPO_URL/q_REPO_BRANCH using printf -v ...
%q or an equivalent safe-quoting routine) and use the escaped names when
constructing export_header and when substituting into modified_func so the eval
only executes safe, quoted strings rather than attacker-controlled content.

---

36-38: ⚠️ Potential issue | 🟠 Major

sed-rewriting build_container is brittle without post-check validation.

The override silently falls back to upstream behavior if the sed pattern no longer matches. Add an immediate verification after each override and fail fast if replacement did not apply.

🧩 Proposed hardening

   modified_func=$(echo "$original_func" | sed "s|bash -c \"\$(curl -fsSL [^\"]*)|bash -c \"${export_header} \$(curl -fsSL ${REPOS_URL}/install/proxmox/install/\${var_install}-install.sh)|g")
   eval "$modified_func"
+  new_func=$(declare -f build_container)
+  if ! grep -Fq "${REPOS_URL}/install/proxmox/install/\${var_install}-install.sh" <<< "$new_func"; then
+    msg_error "Failed to override build_container for REPOS_URL mirror."
+    exit 1
+  fi
 fi
@@
   replacement="${export_header} pct push \"\$CTID\" \"$LOCAL_INSTALLER\" /root/install.sh && lxc-attach -n \"\$CTID\" -- bash /root/install.sh"
   eval "$(echo "$original_func" | sed "s|lxc-attach.*install/\${var_install}.sh.*|$replacement|")"
+  new_func=$(declare -f build_container)
+  if ! grep -Fq "pct push \"\$CTID\" \"$LOCAL_INSTALLER\" /root/install.sh" <<< "$new_func"; then
+    msg_error "Failed to override build_container for LOCAL_INSTALLER."
+    exit 1
+  fi
 fi

Also applies to: 47-50

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@install/proxmox/ct/netalertx.sh` around lines 36 - 38, The sed-based override
of build_container is brittle because if the sed pattern doesn't match it
silently leaves the function unchanged; after creating modified_func (and the
second override at the other occurrence), immediately verify the replacement
actually applied (for example by comparing modified_func to original_func or
grepping for the injected ${export_header} / ${REPOS_URL} token) and if the
check fails log a clear error message and exit non‑zero to fail fast; apply this
verification for the build_container override at the current block and the
similar block at the other occurrence (lines 47-50) and ensure eval is only run
when the verification passes.

---

42-42: ⚠️ Potential issue | 🟠 Major

Fix LOCAL_INSTALLER path (currently resolves to install/install/...).

From install/proxmox/ct, Line 42 currently builds an extra install path segment and can skip the local-installer override unintentionally.

📌 Proposed fix

-LOCAL_INSTALLER="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)/../../install/${NSAPP:-netalertx}-install.sh"
+LOCAL_INSTALLER="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)/../install/${NSAPP:-netalertx}-install.sh"

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@install/proxmox/ct/netalertx.sh` at line 42, LOCAL_INSTALLER currently
appends an extra /install segment causing paths like install/install/...; change
the concatenation so it targets the installer file in the top-level install
directory from the script's location by removing the redundant "/install"
suffix—update the LOCAL_INSTALLER assignment (the variable name LOCAL_INSTALLER
in the install/proxmox/ct script) to use
.../../../${NSAPP:-netalertx}-install.sh instead of
.../../../install/${NSAPP:-netalertx}-install.sh so the override works
correctly.

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro